NETOBSERV-2146: Add the ability to track ownership beyond ReplicaSet #1125
Conversation
|
@leandroberetta: This pull request references NETOBSERV-2146 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
leandroberetta
changed the title
| deployments cache.SharedIndexInformer | ||
| virtualMachineInstances cache.SharedIndexInformer | ||
| virtualMachines cache.SharedIndexInformer | ||
| gateways cache.SharedIndexInformer |
There was a problem hiding this comment.
do we need the Gateway informer here? Currently we have only the RS informer which is sufficient to get the "n+1" hop (the replicaset parents), I'd imagine it could be the same for gateway, having the RS informer being sufficient to get Deployments, so having just Deployment would be sufficient to know about the parent gateway ?
Same question for VM / VMI although I don't know the actual relationships in that case
There was a problem hiding this comment.
It's true about Gateway, not needed so I removed it. Also, I removed support for VirtualMachines but I can re add it.
This is for the Gateway epic but if we want the VirtualMachine stuff, we can add it, although I don't know how to test it yet. What do you think? that can me added in a follow up PR on the custome icons issue?.
|
Review in progress, the overall approach looks good to me 👍 |
Thanks @jotak, I will investigate your questions about the new informers too. |
leandroberetta
force-pushed
the
netobserv-2146
branch
from
4c3b8ac to
7cea2b3
Compare
jotak
added
the
ok-to-test
|
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=5686549 make set-flp-image |
| // Initialize additional informers based on trackedKinds configuration | ||
| for _, kind := range cfg.trackedKinds { | ||
| switch kind { | ||
| case "Deployment", "Gateway": |
There was a problem hiding this comment.
if trackedKinds = ["ReplicaSet", "Deployment", "Gateway"] , then initDeploymentInformer is going to be called twice.
Can't we remove Gateway from this switch, and assume if someone wants to track Gateways they need anyway to also track Deployment ?
There was a problem hiding this comment.
and we could move the ReplicaSet informer creation in this switch, for consistency, no?
There was a problem hiding this comment.
Yes, you're right, thanks for the catch.
There was a problem hiding this comment.
For the ReplicaSet, I would not move it under the switch, because if not trackedkinds are informed (missing the configuration), it would behave as before, just tracking N+1 from the ReplicaSet.
github-actions
bot
removed
the
ok-to-test
|
@leandroberetta: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Description
Adds trackedKinds field in FLP to look for valid owners of resources.
See https://issues.redhat.com/browse/NETOBSERV-2146.
Dependencies
n/a
Checklist
If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.
To run a perfscale test, comment with:
/test flp-node-density-heavy-25nodes